-
Notifications
You must be signed in to change notification settings - Fork 1
Re-implement looking up multiple messages with more flexibility. #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Allow mixing results across multiple topics, with potentially differing access levels. Allow reversing the order of results. Use more efficient paging.
jnation3406
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me. One thing I have noticed is that if you have a topic filter (one or more), then while the initial query is fairly fast (1-2s), each subsequent page load with pagination takes much longer (10-12s). This same slowdown for pagination doesn't seem to appear when no topic filters are present.
| if offset > 0: | ||
| offset -= 1 | ||
| topics = set() | ||
| for record in self.data.values(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like your deleted comment above suggests, this is very inneficient right? Maybe it would be better to cache public topics (and update cached topics on new message ingestion)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh is this just a test fixture - then ignore my comment i guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this code path should only be used by tests.
| else: | ||
| full_clause = None | ||
| if pub_clause is not None and full_clause is not None: | ||
| q = q.where(sqlalchemy.or_(pub_clause, full_clause)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this equally as efficient as combining the topics into one big list and doing a single where topic in clause?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not as efficient, but is required for correctness because the 'public' clause includes different restrictions (topic is one of the public topics, and the message is public). We must not lose that restriction on things with 'public' access level, and don't want to enforce it on things with 'full' access level.
That is not supposed to happen, and I don't think I've seen anything like it in my testing. I'll try to check it again, but do you have any representative API requests for which this occurs? |
Allow mixing results across multiple topics, with potentially differing access levels.
Allow reversing the order of results.
Use more efficient paging.
This is the first phase of updates to support a better REST API/Web UI, which includes all changes that could be made without changes to the data stored in the database.